fix(task-runner-docker)!: refuse a second task while the named one runs - #796
Merged
Conversation
run() removes any container with the configured containerName before starting a task, which is what makes starting one again idempotent: a container left by an earlier run cannot block the next. It did that to a container this runner had started and was still running too, so a second task took the name out from under the first, and the first's wait() failed with nothing to explain why. Nobody hit it while tasks ran one at a time. @lde/sparql-anything now converts chunks in parallel, and Docker is the deployment its concurrency is for, so each chunk would have removed the container of the chunk before it. A name cannot be shared: on a network it is how other containers address the one that has it. So the runner keeps the name for one task at a time and says so, rather than picking a unique name per task, which would leave the address pointing at nothing. Leaving containerName unset runs tasks alongside each other, with Docker naming each container itself. Breaking: a second run() under a name whose task is still running now throws where it used to replace it. Nothing in this repo does that – sparql-qlever shares one named runner between its importer and server, and the pipeline stops the server in a finally before the next dataset – but a consumer relying on replacement will see the error rather than a container disappearing.
The guard added in this branch checked that the name was free and then took it four awaits later, after the pull, the create and the start. Two run() calls that overlap both found it free, and the second went on to remove the first's container – the behaviour the guard exists to prevent, in the only case that produces it. Sequential callers were never the problem. @lde/sparql-anything's pool is exactly such a caller: its workers call run() alongside each other, so with a containerName it would still have killed the first chunk mid-run. The name is now claimed synchronously, before anything is awaited, and given back if the container fails to start. Freeing it is tighter too, in both directions: - wait() frees it once the container has exited, not in a finally. A wait() that fails for a reason of its own – a dropped connection – leaves the container running, and freeing the name would let the next task remove it. - stop() frees it once the container has actually stopped, treating Docker's 304 as the state it asked for. It used to read logs first and free the name in a finally, so a failure to read logs freed the name of a container that was never stopped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The follow-up from #782:
DockerTaskRunnercould not run tasks alongside each other under acontainerName.run()removes any container with that name before starting a task, which is what makes starting one again idempotent – a container left behind by an earlier run cannot block the next. It did that to a container this runner had started and was still running too, so a second task took the name out from under the first, and the first'swait()failed with nothing to explain why.Nobody hit it while tasks ran one at a time.
@lde/sparql-anythingnow converts chunks in parallel, and Docker is the deployment itsconcurrencyoption exists for, so each chunk would have removed the container of the chunk before it.Why not a unique name per task
That was the obvious fix, and it is wrong here: on a
network,containerNameis how other containers address this one –sparql-qleverbuilds its query endpoint out of it. Naming each task's container uniquely would leave that address pointing at nothing.A name belongs to one container at a time, so the runner now keeps it that way and says so. It claims the name synchronously, before the pull and the create – a check that took the name four awaits later would be passed by both of two overlapping calls, which is precisely the case a pool produces:
Unset, Docker names each container itself and tasks run alongside each other with nothing to take from one another – which is what
@lde/sparql-anythingneeds forconcurrency, and its docs now say so.Freeing the name
wait()frees it once the container has exited, andstop()once it has actually stopped, treating Docker's 304 as the state it asked for. Neither does it in afinally: await()that fails for a reason of its own – a dropped connection – leaves the container running, and astop()whose log read fails never stopped anything. Freeing the name in those cases would let the next task remove a container that is still doing its work.Breaking
A second
run()under a name whose task is still running throws, where it used to replace it.Nothing in this repo does that:
sparql-qlevershares one named runner between its importer and its server, the importer awaits each index task, and the pipeline stops the server in afinallybefore the next dataset – so the name is always free again. A consumer that relied on replacement will now see the error instead of a container quietly disappearing.